mkString fixes (mostly moved from the multi-decl overhaul) #714
Conversation
Back when the multi-decl overhaul (#657) switched all unchanged multi-decl members from Decl::print to mkString, these fixes were needed to prevent 3C's output from getting worse on some unchanged multi-decl members in the regression tests. The fixes are no longer needed for that reason, but Mike still asked me to submit them. They do benefit some more complex cases that occur in the regression tests, so the updates to the expected output of those tests provide test coverage for the fixes.
the changes even though they seem about as ad-hoc as the existing code. https://correct-computation.slack.com/archives/G01GKGKHMFD/p1626879436256100?thread_ts=1626818406.255000&cid=G01GKGKHMFD
There was a problem hiding this comment.
This adds special cases to existing code that improves the regression tests, so great job there. The block comment is out of place and needs to be consistent with the rest of the function.
Simplify the last paragraph and move it to the top of the function because it applies generally.
Convert the rest to 1-3 line comments directly referring to a line of code and place them above it.
General knowledge that you deem important can be moved to issue #703 to assist in the overhaul of the function.
As I dug into the reasons why all the conditionals in the PR so far were needed under the current design of the rest of mkString, I realized that the design could be straightened out a bit and correctly handle one more case involving checked pointers to fixed-size arrays. This doesn't address the more difficult problems with wild pointers to fixed-size arrays or with wild pointer levels being in reverse order in general (see #703).
|
After struggling to break down the block comment into smaller pieces (the logic was very interdependent and I couldn't find a way to explain it adequately in 3-line units associated with individual lines of code), I ended up finding a way to simplify the design so that I didn't need such a long comment to explain it. 🙂 |
Any array levels still pending when we reach a function pointer should become part of the function pointer declarator even if some text for an outer checked pointer level has already been added to Ss. However, any array levels that are outside the checked pointer level and have already been committed to EndStrs should remain at the end of the entire type and not get sucked into the function pointer declarator. To maintain this distinction, we need to use a separate `FptrEndStrs` list for the array levels in the function pointer declarator.
Also update a link to go to an existing issue John pointed out.
There was a problem hiding this comment.
I addressed John's comments, updated an issue link to go to #161 (thanks John), and made a bonus fix related to function pointers.
There was a problem hiding this comment.
You were able to refactor and remove some variables. What was the key insight that allowed the change?
I'm assuming we're focusing on this commit; I think each of the other comments is reasonably easy to understand or sufficiently documented by itself.
Here's as much of my thought process as I can reconstruct: After I traced through the execution of the code on the various examples described in the original comment, I realized that the reason I originally needed a special case for function pointers was so that the array levels would be transferred from ConstArrs to EndStrs in time for the if (FV) code block. There was already a conditional addArrayAnnotations call near there, and if I made it unconditional, I probably wouldn't need function pointers to do the addArrayAnnotations where I originally added it. So I looked at what else broke when I made that change, and I found out that the name ended up before the array levels in some cases. Swapping the order of emission of the name and the EndStrs was sufficient to fix that. The rest of the commit was mostly a matter of simplifying conditional expressions and deleting code that I confirmed was no longer needed.
Really, the proof is in the pudding: the regression tests pass, and I've left mkString simpler than I found it. I don't think it's my responsibility in this PR to try to explain or rationalize all of the existing mkString code.
Great, now I can understand the refactor as centering around
I assumed as much, most of the time people remember to check before posting.
That goes without saying. I hope you didn't have a reason to think that it was asked of you.
But code comments are not documentation, at least, I've never used them as such, except for the blocks before functions that show up in automated tools. Perhaps this is a topic we should discuss as a group. |
There was a problem hiding this comment.
The refactor looks fine. Mike agreed that the comment seemed out of place but in such a long, complex function it doesn't distract much. So I'll let that go and that was everything left holding this PR back for me.
If you think you now understand enough about mkString to leave the longer comment above as Mike suggested, I'd encourage you to do that before merging.
I improved the comment on |
mattmccutchen-cci
requested review from
kyleheadley
and removed request for
kyleheadley
Most of these fixes are ones that were previously needed in #657 when it switched all unchanged multi-decl members from
Decl::printtomkString, in order to prevent 3C's output from getting worse on some unchanged multi-decl members in the regression tests. The fixes are no longer needed for that reason, but Mike still asked me to submit them. See the commit messages for additional information.This PR addresses several of the sub-issues of #703.